Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use yarn 3.6.3 #788

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use yarn 3.6.3 #788

wants to merge 4 commits into from
+3,220 −1,634

Conversation

wch
Copy link
Collaborator

@wch wch commented Sep 6, 2023

bslib currently uses whichever version of yarn is installed on the system. This PR updates it to use a fixed version of yarn, so that the behavior is predictable across systems. (This is how things are set up for the https://github.com/rstudio/shiny repository.)

@wch wch requested review from cpsievert and gadenbuie September 6, 2023 19:38
@gadenbuie
Copy link
Member

@cpsievert Do you know if this --production flag is needed?

bslib/tools/yarn_install.R

Line 21 in 6797df4

with_dir("inst", system("yarn install --production"))

We can apparently get similar behavior using yarn workspaces focus --production, but that requires the workspaces plugin and https://yarnpkg.com/cli/workspaces/focus seems to indicate that it's marginally useful.

@cpsievert
Copy link
Collaborator

Do you know if this --production flag is needed?

IIRC it's there so we can essentially rename inst/node_modules to inst/lib without having to explicitly pick which dependencies to keep. If we can do that in a smarter way, I'd definitely be willing to consider another approach

gadenbuie
gadenbuie reviewed Sep 6, 2023
View reviewed changes
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After spending a bit trying to get this to work, I don't think it's currently worth the effort, unless someone else has an easy solution for asking yarn to install just the development dependencies and have them installed into inst/node_modules reliably.

In short, this PR moves bslib from classic yarn to modern yarn, which has more downstream changes than I think we want to take on right now. I think we should table this for now and revisit this post-conf.

@wch
Copy link
Collaborator Author

wch commented Sep 6, 2023

The problem I had was that when I ran yarn (version 1.22.17), it errored out because it apparently didn't know how to get the commit of rstudio/shiny specified in yarn.lock. I managed to get it working locally with the change in this PR, but I'm fine with waiting until post-conf before we merge it.

@wch wch mentioned this pull request Oct 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gadenbuie gadenbuie gadenbuie left review comments

@cpsievert cpsievert Awaiting requested review from cpsievert

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wch @gadenbuie @cpsievert